Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ast): Rename location to loc in AST #664

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ast): Rename location to loc in AST #664

wants to merge 1 commit into from

Conversation

sjbarag
Copy link
Owner

@sjbarag sjbarag commented Jun 30, 2021

BREAKING CHANGE: There's nothing inherently wrong with having AST node locations in a property called node, but it does break compatibility with more common ASTs like the estree standard and makes integration with tools that understand estree significantly more difficult. Rename location to loc in the entire brs syntax tree to improve estree compatibility and enable more advanced eslint rules.

@sjbarag
fix(ast): Rename location to loc in AST
377218d 
BREAKING CHANGE: There's nothing inherently wrong with having AST node
locations in a property called `node`, but it does break compatibility
with more common ASTs like the `estree` standard and makes integration
with tools that understand `estree` significantly more difficult.
Rename `location` to `loc` in the entire `brs` syntax tree to improve
`estree` compatibility and enable more advanced `eslint` rules.
@sjbarag sjbarag added enhancement Any addition to this project's existing capabilities lexer Affects this project's lexer (aka scanner) parser Affects this project's token parser interpreter Affects this project's tree-walking interpreter labels Jun 30, 2021
@sjbarag
Copy link
Owner Author

sjbarag commented Jun 30, 2021

This will require a few small changes in https://github.com/hulu/roca when this lands. If we'd already hit 1.0 I'd say this requires 2.0.0, buuuuuuuuut we haven't. Once I add range support to those nodes (luckily just an additive change) we might be able to consider a 1.0 release 🤔

@sjbarag
Copy link
Owner Author

sjbarag commented Jul 1, 2021 •
edited
Loading

Either way, this probably warrants a -rc release.

alimnios72
alimnios72 approved these changes Jul 29, 2021
View reviewed changes
Copy link
Collaborator

@alimnios72 alimnios72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was intimidated by the huge amount of files change but it was actually pretty easy to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alimnios72 alimnios72 alimnios72 approved these changes

@lkipke lkipke Awaiting requested review from lkipke

@strattonbrazil strattonbrazil Awaiting requested review from strattonbrazil

@heftyfunseeker heftyfunseeker Awaiting requested review from heftyfunseeker

@kirkyoder kirkyoder Awaiting requested review from kirkyoder

Assignees
No one assigned
Labels
enhancement Any addition to this project's existing capabilities interpreter Affects this project's tree-walking interpreter lexer Affects this project's lexer (aka scanner) parser Affects this project's token parser
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sjbarag @alimnios72